Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codeql-action/start-proxy #2376

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Add codeql-action/start-proxy #2376

merged 5 commits into from
Jul 24, 2024

Conversation

aibaars
Copy link
Collaborator

@aibaars aibaars commented Jul 17, 2024

This pull request adds a new experimental action, codeql-action/start-proxy, which starts the HTTP proxy server that is also used by dependabot update jobs. The Action is used for an experimental implementation leveraging Dependabot's configurations and secrets by CodeQL Default Setup jobs that require access to private maven repositories.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@aibaars aibaars added the Update dependencies Trigger PR workflow to update dependencies label Jul 18, 2024
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Jul 18, 2024
Copy link
Contributor

Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks.

@aibaars aibaars marked this pull request as ready for review July 18, 2024 12:02
@aibaars aibaars requested a review from a team as a code owner July 18, 2024 12:02
@aibaars aibaars marked this pull request as draft July 18, 2024 12:02
@aibaars aibaars marked this pull request as ready for review July 19, 2024 14:21
angelapwen
angelapwen previously approved these changes Jul 23, 2024
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me 👍 added a few in-line comments.

Additional nit: could you make all the variables in the TS files camelCase to match the repo convention? Thanks 🙇

@@ -0,0 +1,22 @@
name: "CodeQL: Start proxy"
description: "Start HTTP proxy server"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: "Start HTTP proxy server"
description: "[Experimental] Start HTTP proxy server"

to match the other experimental actions.

},
{
name: "organizationName",
value: "GitHub ic.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gut check: is this meant to be GitHub inc or something like that? I wasn't able to find the expected value in our internal issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted, it comes from the following place, which has the same spelling mistake ;-)

https://github.com/github/dependabot-action/blob/5d0fcfd7688762c2f00f2c9176acc5b185ec6a60/src/proxy.ts#L21-L23

});
subprocess.on("exit", (code) => {
if (code !== 0) {
port = Math.floor(Math.random() * (65535 - 49152) + 49152);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: where does this rand calculation come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to write port = random_int_between(49152, 65535), but I could not find such a function in typescript. The range [49152 .. 65535] is the dynamic ports range, meant for temporary/client stuff. The randomness is to avoid collisions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it — I wasn't sure why those numbers were chosen but makes sense that it's the range for dynamic ports 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-pilot generated the following comment for the code:

// If the proxy failed to start, try a different port from the ephemeral range [49152, 65535]

);
subprocess.unref();
if (subprocess.pid) {
core.saveState("proxy-process-pid", `${subprocess.pid}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about saveState and getState 👍

`start-proxy post-action step failed: ${wrapError(error).message}`,
);
}
if (core.isDebug()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to use config.debugMode instead here; we use this throughout to take into account the debug input parameter in the init action:

debugMode: getOptionalInput("debug") === "true" || core.isDebug(),

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Note, the start proxy action may have failed before the init step has run, so I added some code that tries to get the config and falls back on core.isDebug() if it is undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

@angelapwen
Copy link
Contributor

We may want to add a changelog note indicating this is a new experimental action, probably something similar to this one resolve-environment: https://github.com/github/codeql-action/blob/main/CHANGELOG.md#2201---21-jun-2023.

angelapwen
angelapwen previously approved these changes Jul 24, 2024
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all the comments — looks great to me!

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@aibaars aibaars merged commit f67c9cd into main Jul 24, 2024
310 checks passed
@aibaars aibaars deleted the aibaars/start-proxy branch July 24, 2024 12:26
@henrymercer
Copy link
Contributor

@angelapwen When we do the next release, we'll probably want to add a commit to the v3 -> v2 backport PR to run this Action on Node 16. I wonder whether it's worth doing a release now so we don't forget — what do you think?

@angelapwen
Copy link
Contributor

Good point @henrymercer — I'll release earlier in the workday tomorrow.

@angelapwen
Copy link
Contributor

Kicking off the release now 🙏

@angelapwen
Copy link
Contributor

Done in #2390!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants